Skip to content

Small performance optimization in lz4 compressor implementation #1910

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Small performance optimization in lz4 compressor implementation #1910

wants to merge 1 commit into from

Conversation

@joao-r-reis
Copy link
Contributor

@joao-r-reis joao-r-reis commented Sep 23, 2025

Ran some benchmarks to compare 2.0.0-rc1 with 1.7.0 and noticed a 2-3% degradation when compression was enabled. I made these quick optimizations, re-ran the tests and the degradation went away (0-1%).

Benchmark code here (first branch, will eventually merge it)

Results here in the "compression fix test" tab.

@joao-r-reis joao-r-reis changed the title Small performance optimization in lz4 compressor implementation. Small performance optimization in lz4 compressor implementation Sep 23, 2025
@joao-r-reis
Small performance optimization in lz4 compressor implementation.
ece2ca7 
Patch by João Reis; reviewed by TBD for CASSGO-90
worryg0d
worryg0d approved these changes Sep 23, 2025
View reviewed changes
Copy link
Contributor

@worryg0d worryg0d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I didn't expect the copy function call on a zero-length source to cost that much... But well, it is still a call, so it makes sense. I assume the same applies to the zero value slicing thing...

Also, this is why I don't like to make such big patches with a lot of different areas involved. It takes a lot of time to identify such things.

Anyway, thanks @joao-r-reis! You've done a really great job with this one. If it were me, I would spend much more time trying to identify this. On the other hand, this is a good lesson for me

@joao-r-reis
Copy link
Contributor Author

joao-r-reis commented Sep 24, 2025 •
edited
Loading

Yeah honestly I microbenchmarked the grow function alone and the copy call didn't seem to cause much impact if any so I'm not 100% confident that this change actually helped, the 1-2% difference I saw was probably just normal variance but I'm rerunning tests and will try to profile later today to see if I understand exactly why compression looks to be slower now

@joao-r-reis
Copy link
Contributor Author

joao-r-reis commented Sep 24, 2025

Ok after some profiling I decided to go with a different approach to mitigate the performance regression that we are seeing. I'm not convinced the changes on this PR even helped that much anyway so I'm closing this in favor of #1912 (and repurposed the JIRA ticket)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@worryg0d worryg0d worryg0d approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@joao-r-reis @worryg0d